🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs#32
Conversation
Replaces `execSync` with `execFileSync` to properly handle array-based arguments and validate input parameters starting with hyphens. Also sanitizes the internal stack traces exposed to the frontend in case of a crash or failure. Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a command injection vulnerability in the docker-logs API by replacing execSync with execFileSync, implementing input validation to prevent flag injection, and sanitizing error responses to avoid leaking stack traces. Feedback suggests transitioning to the asynchronous execFile to avoid blocking the event loop and to better handle combined stdout and stderr streams, ensuring all logs are captured while preventing system information leaks in error messages.
| const output = execFileSync('docker', ['logs', '--tail', tail, containerId], { stdio: ['pipe', 'pipe', 'pipe'], encoding: 'utf8' }); | ||
| logs = output.trim().split('\n'); | ||
| } catch (err: any) { | ||
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||
| if (err.stderr) { | ||
| logs = err.stderr.toString().trim().split('\n'); | ||
| } else { | ||
| throw err; | ||
| throw new Error("Erreur d'exécution de la commande"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation has two significant issues:
- Missing Logs: Using
execFileSyncwith separate pipes forstdoutandstderrmeans that any logs written tostderrby the containerized application are ignored on success. - Information Leak: If the command fails, the
catchblock returns the rawstderrfrom the Docker CLI (lines 39-40), which can leak internal system details (e.g., "No such container") and contradicts the goal of sanitizing error messages.
Using execFile allows capturing both streams and provides a cleaner way to handle errors uniformly.
const { stdout, stderr } = await execFileAsync('docker', ['logs', '--tail', tail, containerId], { encoding: 'utf8' });
const output = stdout + stderr;
logs = output.trim() ? output.trim().split('\n') : [];
} catch (err: any) {
throw new Error("Erreur d'exécution de la commande");
}| @@ -1,5 +1,5 @@ | |||
| import type { APIRoute } from 'astro'; | |||
| import { execSync } from 'child_process'; | |||
| import { execFileSync } from 'child_process'; | |||
There was a problem hiding this comment.
Switching to the asynchronous execFile is recommended to avoid blocking the Node.js event loop in this API route. This requires importing promisify to handle the callback-based API with async/await.
import { execFile } from 'child_process';
import { promisify } from 'util';
const execFileAsync = promisify(execFile);1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Command injection and potential flag injection due to unsanitized user inputs (
id,tail) concatenated directly into anexecSyncbash string execution.🎯 Impact: Anyone with access to the API could potentially execute arbitrary code or inject flags that affect docker configurations on the host, exposing sensitive environments or escalating privileges.
🔧 Fix: Replaced
execSyncwithexecFileSyncrunning an exact['logs', '--tail', tail, containerId]argument array, removing the shell-dependency. I also added explicit checks to block hyphens (-) to avoid flag injection. Error messages are now safely sanitized, masking original traces.✅ Verification: Tests continue to pass, pre-commit validation confirmed no type issues directly matching the change. Tested manually through local test runners.
PR created automatically by Jules for task 10124517214004022915 started by @bobdivx